Refactor Go-DI template generation to use location expressions #31424
Conversation
grantseltzer
added
team/dynamic-instrumentation
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: e49efd5 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +2.56 | [-0.43, +5.55] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | +1.18 | [+1.05, +1.31] | 1 | Logs bounds checks dashboard |
| ➖ | otel_to_otel_logs | ingress throughput | +1.01 | [+0.33, +1.69] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api_cpu | % cpu utilization | +0.43 | [-0.29, +1.16] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | +0.10 | [-0.66, +0.87] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency_http2 | egress throughput | +0.09 | [-0.71, +0.89] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | +0.05 | [-0.60, +0.70] | 1 | Logs |
| ➖ | file_to_blackhole_300ms_latency | egress throughput | +0.05 | [-0.58, +0.68] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | +0.04 | [-0.80, +0.87] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.00 | [-0.01, +0.02] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | +0.00 | [-0.11, +0.11] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency_http1 | egress throughput | -0.05 | [-0.96, +0.86] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.16 | [-0.94, +0.62] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency_linear_load | egress throughput | -0.23 | [-0.69, +0.22] | 1 | Logs |
| ➖ | quality_gate_idle | memory utilization | -0.24 | [-0.28, -0.19] | 1 | Logs bounds checks dashboard |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | -0.53 | [-0.60, -0.46] | 1 | Logs |
| ➖ | file_tree | memory utilization | -0.55 | [-0.67, -0.43] | 1 | Logs |
Bounds Checks: ❌ Failed
| perf | experiment | bounds_check_name | replicates_passed | links |
|---|---|---|---|---|
| ❌ | file_to_blackhole_100ms_latency | lost_bytes | 5/10 | |
| ❌ | file_to_blackhole_0ms_latency | lost_bytes | 9/10 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http1 | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http1 | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http2 | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http2 | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency_linear_load | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | |
| ✅ | quality_gate_idle | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | lost_bytes | 10/10 | |
| ✅ | quality_gate_logs | memory_usage | 10/10 |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
grantseltzer
force-pushed
the
grantseltzer/DEBUG-2982-multiple-dereference-support-2
branch
2 times, most recently
from
b3edbd7 to
924fb7f
Compare
pkg/dynamicinstrumentation/testutil/sample/kafka_go_forwarder/kafka_go_forwarder
Outdated
Show resolved
Hide resolved
grantseltzer
force-pushed
the
grantseltzer/DEBUG-2982-multiple-dereference-support-2
branch
2 times, most recently
from
e1ffc3a to
3869f77
Compare
Package size comparisonComparison with ancestor Diff per package
Decision |
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv aws.create-vm --pipeline-id=50960741 --os-family=ubuntuNote: This applies to commit 26663d4 |
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
This is a major refactor of the way Go-DI generates bpf programs. Instead of generating bpf programs via templates for specific types, it breaks down captures into basic building blocks of operations such as reading from registers/stack, dereferencing, adding offsets, writing to output, etc... Templates are now these basic operations and as a result we can express much more complex captures, such as pointers to pointers, slices of strings or any combination of types that were previously supported. Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
…instead of right before the values. Still need to update event parsing code Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
This changes the logic for collection limit labels to create a new label before every slice element and jump to it accordingly. This is to avoid collissions such as in the case of embedded slices. Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
| } | ||
| for i := range parameters { | ||
| if i >= len(funcMetadata.Parameters) { | ||
| return errors.New("parameter metadata does not line up with parameter itself") |
There was a problem hiding this comment.
Is it worth including information about the extra parameter in the error?
Should we consider the analysis as failed when this doesn't line up or just take the parameters we have?
If we fail here we can have the length check outside of the inner loop.
There was a problem hiding this comment.
You bring up a good point. As you also pointed out, the error wasn't being checked anyway. It all relates to a larger comprehensive approach to how we want to handle errors that occur for instrumenting a single parameter, or portion of a single parameter to be fault tolerant. I've changed the returned error to be logs so they're at least surfaced and attempt to continue generating location expressions. I don't expect the errors to occur here as if there was an issue with DWARF it'd be more likely an error would occur earlier in execution. Regardless it's something that can come up and we should handle as part of a larger resiliency strategy.
| @@ -80,6 +81,7 @@ func AnalyzeBinary(procInfo *ditypes.ProcessInfo) error { | |||
| // Use the result from InspectWithDWARF to populate the locations of parameters | |||
| for functionName, functionMetadata := range r.Functions { | |||
| putLocationsInParams(functionMetadata.Parameters, r.StructOffsets, procInfo.TypeMap.Functions, functionName) | |||
| populateLocationExpressions(r.Functions, procInfo) | |||
There was a problem hiding this comment.
We are ignoring the error here
There was a problem hiding this comment.
See comment you made in the function body.
| ExpectedParameters []*ditypes.Parameter | ||
| }{ | ||
| { | ||
| FuncName: "github.com/DataDog/datadog-agent/pkg/dynamicinstrumentation/testutil/sample.test_single_int", |
There was a problem hiding this comment.
You can add more complex tests here.
| log.Tracef("event dropped by rate limit. Probe %s\t(%d dropped events out of %d)\n", | ||
| return nil, log.Errorf("event dropped by rate limit. Probe %s\t(%d dropped events out of %d)\n", |
There was a problem hiding this comment.
This will create a lot of noise in the logs, we should not error log every rate limited event.
There was a problem hiding this comment.
The error is logged at the trace level (for normal events, error level for config), do you think that's too noisy still?
| if len(buffer) > bufferIndex+int(paramDefinition.Size) { | ||
| paramDefinition.ValueStr = string(buffer[bufferIndex : bufferIndex+int(paramDefinition.Size)]) |
There was a problem hiding this comment.
This is incorrect, it will lose data if the size aligns exactly to the size of the buffer. It should be len(buffer) >= bufferIndex+int(paramDefinition.Size), right?
There are multiple places in this file with the same problem.
There was a problem hiding this comment.
I'm sure this has never happened in our testing of DI but you were correct, good catch 👍 I'll write a test case as well.
I misremembered if Go would panic if you slice with the length of the slice.
i.e.:
x := []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 9}
fmt.Println(len(x), x[:10]) // ok
fmt.Println(x[10]) // not ok
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
|
[Fast Unit Tests Report] On pipeline 51750040 (CI Visibility). The following jobs did not run any unit tests: Jobs:
If you modified Go files and expected unit tests to run in these jobs, please double check the job logs. If you think tests should have been executed reach out to #agent-devx-help |
Signed-off-by: grantseltzer <[email protected]>
grantseltzer
force-pushed
the
grantseltzer/DEBUG-2982-multiple-dereference-support-2
branch
from
b2892a3 to
df484f3
Compare
Signed-off-by: grantseltzer <[email protected]>
…o include key type. Use macros in DI Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
grantseltzer
force-pushed
the
grantseltzer/DEBUG-2982-multiple-dereference-support-2
branch
from
be4efa2 to
1f64aff
Compare
Signed-off-by: grantseltzer <[email protected]>
Signed-off-by: grantseltzer <[email protected]>
grantseltzer
force-pushed
the
grantseltzer/DEBUG-2982-multiple-dereference-support-2
branch
from
224e16a to
f0b61fb
Compare
…s are assigned Signed-off-by: grantseltzer <[email protected]>
What does this PR do?
This is a major refactor of the way Go-DI generates bpf programs. Instead
of generating bpf programs via templates for specific types, it breaks
down captures into basic building blocks of operations such as reading
from registers/stack, dereferencing, adding offsets, writing to output,
etc...
Templates are now these basic operations and as a result we can express
much more complex captures, such as pointers to pointers, slices of strings
or any combination of types that were previously supported.
Motivation
The previous iteration of Go-DI failed to be able to express complex types such as pointers to pointers. We didn't have a way of expressing any amount of multiple dereferences. This greatly increases the supported features/types for our customers to instrument.
Describe how to test/QA your changes
Run e2e tests
Possible Drawbacks / Trade-offs
Additional Notes